Switch semantics of filterLongerThan and filterShorterThan #1587
Conversation
JoelCourtney
had a problem deploying
to
e2e-test
GitHub Actions
Failure
Mythicaeda
added
the
breaking change
|
I think the naming of these functions needs to be revisited, as if I use |
This PR is the revisitation 🙂
I disagree, I think the proposed change is more intuitive & closer to standard - as @JoelCourtney notes, it makes it more consistent with things like |
There was a problem hiding this comment.
I asked a few more people for opinions on this today (@mattdailis @joswig and @jmorton ) to try and reach consensus, and where we landed was:
filterXin general can be a confusing name, in future we should consider naming things like thiskeepXorremoveX- However, doing ^this would a more far-reaching change than this PR needs right now & would take some more time/buy-in
- Given the choice between
filterXmeaning "keep X" or "filter out all X's", everyone I asked thought that "keep X" was the more intuitive choice given the syntax offilter()mentioned above - Therefore I think we should approve & merge this PR
dandelany
force-pushed
the
fix/switch-filter-by-duraitons
branch
from
0c948e7 to
3801157
Compare
When I made the
filterShortThanandfilterLongerThanmethods in the timeline library, I had a choice of semantics, either "filterShorterThan filters OUT intervals shorter than" or "filterShorterThan filters SUCH THAT all intervals are shorter than". I chose the first, but in practice I get confused every time I use these functions. This is because with all other filter-like functions such ashighlightEqualToorisolateTrue, I named them in the pattern<operation><predicate>. So inhighlightEqualTo("x"), if the segment is equal to"x", it is highlighted. Unfortunately I broke that pattern withfilterLonger/ShorterThan; infilterShorterThanthe object is retained if it doesn't match the shorter-than predicate.I understand that this is the absolute worst kind of breaking change. It doesn't just invalidate existing code, it completely reverses what it does. But I can't think of another way that isn't more work than it's worth. I'm hoping the impact is pretty small since procedural scheduling is still a new feature.